fix(balances): materialize entries dated before the opening anchor - #2434
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 703e7aaf756901881eaefacc5d2698d1a24f97a5 and 89584b8. 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds a ChangesPre-anchor Balance Materialization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07d39a50fe
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
93480c7 to
703e7aa
Compare
jjmata
left a comment
There was a problem hiding this comment.
The root-cause analysis and fix are solid — bounding on min(opening_anchor_date, oldest_entry_date) is exactly the right model, and the three regression tests cover the important paths (forward seed, reverse loop, incremental purge).
One thing worth considering: calculation_start_date fires an extra entries.minimum(:date) query every time it's called, and within a single sync it's called more than once. Looking at materializer.rb, purge_stale_balances references it on two code paths (the early-return incremental branch and the oldest_valid_date assignment), and the forward/reverse calculators call it via calc_start_date/downto. That's 3–4 extra queries per sync.
A simple memoization would eliminate the redundant hits:
def calculation_start_date
@calculation_start_date ||= [ account.opening_anchor_date, account.entries.minimum(:date) ].compact.min
endSince BaseCalculator instances are per-sync and not reused, there's no staleness concern. Worth doing before this lands, especially for accounts with large entry histories where MIN(date) is a non-trivial scan.
Everything else looks great — the return [0, 0] seed guard in opening_starting_balances is clean, the downto(calculation_start_date) bound in the reverse calculator is minimal, and the public visibility with a doc comment is the right call for the materializer to share the same lower bound.
Generated by Claude Code
703e7aa to
89584b8
Compare
|
Thanks for the review! Applied the memoization in the latest push: def calculation_start_date
@calculation_start_date ||= [ account.opening_anchor_date, account.entries.minimum(:date) ].compact.min
endAgreed on the reasoning — calculator instances are per-sync so there's no staleness risk, and this collapses the 3–4 |
|
@jjmata if all ok, i suggest to merge |
Problem
Manual Valuation/reconciliation entries dated before an account's
opening_anchorare stored but never materialized intoBalancerows, so the net-worth chart is empty for that period and a normal sync never backfills it.Root cause: both balance calculators use
account.opening_anchor_dateas the hard lower bound of iteration:Balance::ForwardCalculator#calc_start_date→account.opening_anchor_dateBalance::ReverseCalculator→current_anchor_date.downto(account.opening_anchor_date)and
Account::OpeningBalanceManager#opening_datereturns the opening-anchor valuation's date whenever one exists. The system intends the opening anchor to sit at/before the oldest entry (set_opening_balanceeven validates this), but the reconciliation path (create_reconciliation) does not move the anchor back when a valuation is created with an earlier date. The invariant breaks, and every entry before the anchor is silently clipped out of the series.How to reproduce
opening_anchorat, say, today).account.balanceshas no row for that date → net-worth chart flat/empty before the anchor.Fix
Bound the calculation window on
min(opening_anchor_date, oldest_entry_date)instead of the anchor date alone (new sharedBalance::BaseCalculator#calculation_start_date).*_adjustmentsfield is affected — later totals are correct.downtobound;use_opening_anchor_for_date?still keys off the anchor's real date, so the anchor's own treatment is unchanged and pre-anchor reconciliation waypoints reset normally.No data migration and no anchor mutation — purely a calculation-window fix, so it heals existing accounts on the next sync.
Tests
forward_calculator_test.rb: "materializes entries dated before the opening anchor" — reconciliation before the anchor is materialized; window starts at the earliest entry; balances correct across the anchor.reverse_calculator_test.rb: "materializes reconciliation waypoints dated before the opening anchor".Summary by CodeRabbit